Skip to content

update comment on MinimumChainWork check #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 93 commits into
base: master
Choose a base branch
from

Conversation

willcl-ark
Copy link
Contributor

test

l0rinc and others added 30 commits October 19, 2024 18:49
Also separated the roundtrip testing from the random string decoding for clarity

Note that while BIP 173 claims:
```
The human-readable part, which is intended to convey the type of data, or anything else that is relevant to the reader. This part MUST contain 1 to 83 US-ASCII characters, with each character having a value in the range [33-126]. HRP validity may be further restricted by specific applications.
```
bech32::Encode rejects uppercase letters.
Move the implementation (method definitions) from `test/util/net.h` to
`test/util/net.cpp` to make the header easier to follow.
This change allows the entire `depends/<host_prefix>` directory to be
relocatable.
1. Check that outbound nodes are treated
the same as whitelisted connections for
the purposes of getdata delays

2. Add test case that demonstrates
download retries are preferentially
given to outbound (preferred) connections
even when multiple announcements are
considered ready.
…lass

This allows reusing them in other mocked implementations.
…o it

And also allows gradually providing the data to be returned by `Recv()`
and sending and receiving net messages (`CNetMessage`).
`get_socket_inodes()` calls `os.listdir()` and then iterates on the
results using `os.readlink()`. However a file may disappear from the
directory after `os.listdir()` and before `os.readlink()` resulting in a
`FileNotFoundError` exception.

It is expected that this may happen for `bitcoind` which is running and
could open or close files or sockets at any time. Thus ignore the
`FileNotFoundError` exception.
In almost all cases (the only exception is `getifaddrs`), we know the
size of the data passed into SetSockAddr, so we can check this to be
what is expected.
This adds a NodeSteadyClock, which is a steady_clock that can be mocked
with millisecond precision.
This will be needed for the test harness.
Add a mock for a simple scriptable UDP server, and use this to test
various code paths (including successful mappings, timeouts and errors)
in the PCP and NATPMP implementations.
p2p_ibd_txrelay expects no GETDATA to have been received by a peer after
announcing a transaction. The reason is that the node is doing IBD, so
transaction requests are not replied to. However, the way this is checked
is wrong, and the check will pass even if the node **was not** in IBD.

This is due to the mocktime not being properly initialized, so the check
is always performed earlier than it should, making it impossible for the
request to be there
…ve sent a message

p2p_orphan_handling checks whether a message has not been requested slightly
too soon, making the check always succeed. This passes unnoticed since the
expected result is for the message to not have been received, but it will make
the test not catch a relevant change that should make it fail
A bpftrace script that logs information from the
net:*_connection tracepoints.

I've tested this script with bpftrace version 0.14.1 and v0.20.2.
A v3 onion address with a `:` and a five digit port has a length of
68 chars. As noted in
bitcoin/bitcoin#25832 (comment)
peers e.g. added via hostname might have a longer CNode::m_addr_name.
These might be cut off in tracing tools.
- This commit renamed coinbase_max_additional_weight to block_reserved_weight.

- Also clarify that the reservation is for block header, transaction count
  and coinbase transaction.
- The reserved weight of the coinbase transaction is an estimate and
  may not reflect the exact value; it can be lower.

- It should be clear that `currentblockweight` includes the reserved coinbase transaction weight.
  whereas `currentblocktx` does not account for the coinbase transaction count.

- Also clarify `m_last_block_num_txs` and `m_last_block_weight`
@willcl-ark
Copy link
Contributor Author

Updated the runner so that pyperf system outputs:

[root@nixos:~]# nix-shell -p python313Packages.pyperf

[nix-shell:~]# pyperf system
Show the system configuration
Unit irqbalance.service could not be found.

System state
============

CPU: use 14 logical CPUs: 2-15
Perf event: Maximum sample rate: 100000 per second
ASLR: Full randomization
Linux scheduler: Isolated CPUs (14/16): 2-15
Linux scheduler: RCU disabled on CPUs (14/16): 2-15
CPU Frequency: 0-15=min=400 MHz, max=5389 MHz
IRQ affinity: Default IRQ affinity: CPU 0-15
IRQ affinity: IRQ affinity: IRQ 0-15,26-31,39-40,42,44-52,54-61,63-78,112,114-119=CPU 0-15; IRQ 79,95=CPU 0; IRQ 80,96=CPU 1; IRQ 81,97=CPU 2; IRQ 82,98=CPU 3; IRQ 83,99=CPU 4; IRQ 84,100=CPU 5; IRQ 85,101=CPU 6; IRQ 86,102=CPU 7; IRQ 87,103=CPU 8; IRQ 88,104=CPU 9; IRQ 89,105=CPU 10; IRQ 90,106=CPU 11; IRQ 91,107=CPU 12; IRQ 92,108=CPU 13; IRQ 93,109=CPU 14; IRQ 94,110=CPU 15

Advices
=======

Perf event: Set max sample rate to 1

Warnings
========

Turbo Boost (MSR): Failed to read MSR 0x1a0 from /dev/cpu/2/msr: [Errno 5] Input/output error

Run "python3.13 -m pyperf system tune" to tune the system configuration to run benchmarks

Deliberately left perf events cranked up (as we are perf-ing events!)

fanquake and others added 12 commits February 12, 2025 09:47
…scripts

76c0901 guix: remove test-security/symbol-check scripts (fanquake)

Pull request description:

  These scripts are becoming more of nuisance, than a value-add; particularly since we've been building releases using Guix. Adding new (release bin) tests can be harder, because it requires constructing a failing test, which is becoming less easy, e.g trying to disable a feature or protection that has been built into the compiler/toolchain by default.

  In the pre-Guix days, these were valuable to sanity-check the environment, because we were pulling that pre-built from Ubuntu, with little control. At this point, it's less clear what these scripts are (sanity) checking.

  Note that these also weren't completely ported to CMake (#31698), see also #31715 which contains other fixes that would be needed for these test-tests, to accomodate future changes.

ACKs for top commit:
  hebasto:
    ACK 76c0901.
  theuni:
    utACK 76c0901

Tree-SHA512: 99b5e7c0645c6966a45b17f411b5bee61df23c64d8258cce0ad9cdea4c3af4d4db32ca5fd80d0df2a3a30ba873eb772cc0d5901c345ff7f0eea13fcb971443b4
Co-authored-by: David Gumberg <[email protected]>
Co-authored-by: Lőrinc <[email protected]>
Co-authored-by: fanquake <[email protected]>
@willcl-ark willcl-ark force-pushed the 2502_ctx_checks_in_connectblock-v2 branch 2 times, most recently from b107dda to c7c3bc6 Compare February 12, 2025 09:54
marcofleon and others added 6 commits February 12, 2025 09:56
The headers presync logic should be enough to prevent memory DoS using
low-work headers. Therefore, we no longer have any use for checkpoints.
In the following commits we'll make it so all consensus rules are checked when
connecting blocks. However we should not check again there that the block
timestamp is not more than 2 hours in the future, just in case our system clock
went backward.

To achieve this we first split out the future timestamp check in its own function.
We want to check all consensus rules when connecting a block, and therefore to
call CtxCheckBlockHeader in ConnectBlock. But we don't want to perform the future
timestamp check there again in case our system clock went backware. Therefore move
the future timestamp check out of CtxCheckBlockHeader to its callers.
At the moment we only sanity check part of the consensus rules when connecting blocks (only
CheckBlock() is called in ConnectBlock()). This makes it hard to reason about software upgrades
if we were to introduce new consensus rules. For instance a timewarp fix or a BIP34 supplementation
would go into CtxCheckBlock{Header}, and a block invalid according to the new rules could be let in
by an unupgraded version and still be connected by an upgraded one.

These additional checks should not noticeably increase block propagation at tip since their expensive
part (the witness merkle root check) has been cached since 1ec6bbe.
They would however have a performance impact on IBD as the cache is not leveraged there.
@willcl-ark willcl-ark force-pushed the 2502_ctx_checks_in_connectblock-v2 branch from c7c3bc6 to 17b861f Compare February 12, 2025 09:57
Copy link
Contributor

📊 Benchmark results for this run (13282683745) will be available at: https://bitcoin-dev-tools.github.io/benchcoin-testing/results/pr-1/13282683745/index.html after the github pages "build and deployment" action has completed.
🚀 Speedups: mainnet-large: -2.5%

@willcl-ark
Copy link
Contributor Author

{
  "results": [
    {
      "command": "base (61428213e598d8236d73d501007e7d5f9aea57c8)",
      "mean": 634.9448975221601,
      "stddev": 12.32405420843455,
      "median": 637.77037637336,
      "user": 814.24212232,
      "system": 121.65090559999999,
      "min": 617.29642567436,
      "max": 650.45639934136,
      "times": [
        617.29642567436,
        637.77037637336,
        639.46035752336,
        629.74092869836,
        650.45639934136
      ],
      "exit_codes": [
        0,
        0,
        0,
        0,
        0
      ],
      "parameters": {
        "commit": "base"
      }
    },
    {
      "command": "head (c5552a9144cb55dd1c74f896090ddf9ee3674215)",
      "mean": 650.7897488139599,
      "stddev": 4.359595524818315,
      "median": 652.4612346223599,
      "user": 830.9055277199999,
      "system": 120.89877219999998,
      "min": 643.44022050836,
      "max": 654.49167212236,
      "times": [
        643.44022050836,
        654.49167212236,
        653.11161906436,
        650.44399775236,
        652.4612346223599
      ],
      "exit_codes": [
        0,
        0,
        0,
        0,
        0
      ],
      "parameters": {
        "commit": "head"
      }
    }
  ]
}

@l0rinc
Copy link
Contributor

l0rinc commented Feb 12, 2025

Run "python3.13 -m pyperf system tune" to tune the system configuration to run benchmarks

is the only diff compared to pyperf system that Perf event: Set max sample rate to 1?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.